Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better errors for schema merging #607

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tormeh
Copy link

@tormeh tormeh commented Jun 10, 2024

This is a follow-up to #521 . I finally got time to look at it all. I could polish it further without feedback, but at this point all the boilerplate is mostly done and I'm starting to encounter design questions. So I thought it best to submit it for review so I don't fly blind.

@tormeh tormeh changed the title Merge errors slim Better errors for schema merging Jun 10, 2024
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • What are the design questions you've encountered?
  • What would you think about a single Error type rather than several?

As I recall, the goal here is to improve the the debugging story when understanding a merge failure. It looks like the only output that would change is the new log message in the "Subschemas with other stuff" case. Is that right?

typify-impl/src/convert.rs Outdated Show resolved Hide resolved
typify-impl/src/merge.rs Outdated Show resolved Hide resolved
Comment on lines +88 to +92
#[error("Error when trying to merge the two schema objects {a:?} and {b:?}: {source}")]
ObjectSchemaMerge {
source: Box<ObjectSchemaMergeError>,
a: SchemaObject,
b: SchemaObject,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems sort of heavy to clone these SchemaObjects.. will the caller not already have access to them?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but how will you know which two SchemaObjects couldn't be merged if you don't have them in the error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These clones only happen when there's an error. How big are these structs typically?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we use a Result::Err to indicate that the merge was not viable, this isn't an unexpected or aberrant condition. It doesn't lead to a failure to generate code from a schema. It's an expected and typical condition.

typify-impl/src/merge.rs Outdated Show resolved Hide resolved
@tormeh
Copy link
Author

tormeh commented Jun 18, 2024

  • What are the design questions you've encountered?

Mostly how much context to include. In particular, calls to try_merge_with_subschemas have a mutable SchemaObject. If you want to know what this value was when the function is called you have to clone it before the call, so that even if there is no error you're still doing clones for error handling purposes. I opted against this, but I imagine opinions differ on the right thing to do here.

  • What would you think about a single Error type rather than several?

Absolutely a valid choice, although a bit annoying to change now 😅. I chose this approach because it provides the most structure, but it does require a bit of maintenance whenever something is changed... On the other side of this spectrum is stuff like Anyhow, where the error type is essentially a string. I think both have their own charm. A library should IMO never expose stringly errors like Anyhow to applications (because they can't be matched on), but these errors are never shown to the caller, so it's not that important, I guess. My intention with how I've done it is to provide almost a stack trace of what went wrong, along with the data that the different layers were called with. This should provide a great debugging experience.

As I recall, the goal here is to improve the the debugging story when understanding a merge failure. It looks like the only output that would change is the new log message in the "Subschemas with other stuff" case. Is that right?

That's what the code does, yes. There used to be an unwrap where all the errors surfaced by crashing the progenitor. Originally I wanted to print something there, but it seems to be gone now. Would it be interesting to print something somewhere else? Or potentially do something else with the error?

@ahl
Copy link
Collaborator

ahl commented Jun 27, 2024

I appreciate you taking another run at this. I'm not convinced that this is going to be helpful enough in debugging a merge failure to warrant the ongoing burden of the cacophony of errors and variants.

What would you think about a single Error type rather than several?

Absolutely a valid choice, although a bit annoying to change now

Indeed, but it's going to be much more annoying for me to change later.

This should provide a great debugging experience.

Can you show that? Stepping back: could we start from a specific condition that's currently a challenge to debug--I expect there are many--and focus on what would make it easier to debug. If we want those conditions to be easier to debug and stay easier to debug, we'll want tests that preserve the behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants